feat: full GitLab integration — parity with GitHub#1120
Open
jcfs wants to merge 12 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full GitLab integration: backend watch/task models and DB schema, store CRUD and reservation semantics, service APIs (watches, merge, search, sync, cleanup), background poller, HTTP + WebSocket routes (plus mock), orchestrator event wiring and startup, and frontend API, types, Zustand state, and hooks. ChangesBackend GitLab Integration
Frontend State and Data Access
Sequence Diagram(s) sequenceDiagram
participant Client
participant WSDispatcher
participant GitLabService
participant Store
participant Poller
Client->>WSDispatcher: WS action (trigger/list/update)
WSDispatcher->>GitLabService: call service handler
GitLabService->>Store: read/write watches/tasks
Poller->>GitLabService: periodic poll -> discoveries
GitLabService->>Client: publish events / return responses
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Claude finished @jcfs's task in 6m 27s —— View job Code Review
FindingsBlocker (must fix before merge)1. Missing backend tests —
2. Missing frontend tests —
3. Label filter silently dropped when
// current:
filter := watch.CustomQuery // filter = customQuery
if filter == "" {
filter = "assignee_username=..."
}
if len(watch.Labels) > 0 {
filter = filter + "&labels=..." // appended to filter, but...
}
issues, err := client.ListIssues(ctx, filter, watch.CustomQuery)
// buildIssueSearchQuery ignores `filter` ↑ because customQuery != ""
if watch.CustomQuery != "" {
q := watch.CustomQuery
if len(watch.Labels) > 0 {
q += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ","))
}
return client.ListIssues(ctx, "", q)
}
filter := "assignee_username=" + url.QueryEscape(username)
if len(watch.Labels) > 0 {
filter += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ","))
}
return client.ListIssues(ctx, filter, "")4. Watch events are published but no orchestrator subscriber creates tasks —
Suggestion (recommended, doesn't block)5.
6. Redundant
filter := "reviewer_username=" + url.QueryEscape(username)
if watch.CustomQuery != "" {
filter = "" // customQuery overrides; filter is ignored
}
return client.SearchMRs(ctx, filter, watch.CustomQuery)7.
Summary
Verdict: Blocked — fix blockers first. The infrastructure here is well-built (dedup protocol, cleanup policies, action presets, WS handler symmetry with GitHub) but items 1–2 (missing tests) and item 4 (unsubscribed events → no task creation) need to be addressed before this can ship. Item 3 is a logic bug that would confuse users who set both |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
26 issues found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/backend/internal/gitlab/store_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/store_watches.go:170">
P1: Normalize `PollIntervalSeconds` in `UpdateReviewWatch` the same way as create, otherwise updates can persist invalid (<=0) intervals.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store_watches.go:416">
P1: `UpdateIssueWatch` should clamp/default `PollIntervalSeconds`; currently invalid values can be persisted on update.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-status.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-status.ts:18">
P1: The initial-effect guard uses `status` truthiness instead of a loaded marker, so `null` results trigger repeated status refetches (disconnect/error loops).</violation>
</file>
<file name="apps/backend/internal/gitlab/service_cleanup.go">
<violation number="1" location="apps/backend/internal/gitlab/service_cleanup.go:59">
P1: Issue-watch fetch errors are defaulted to `CleanupPolicyAuto`, which can delete issue tasks incorrectly when watch metadata retrieval fails transiently. Erroring watches should be treated as unknown and skipped until a later sweep.
(Based on your team's feedback about treating watch fetch errors as unknown during cleanup sweeps.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/mock_client.go">
<violation number="1" location="apps/backend/internal/gitlab/mock_client.go:440">
P2: `SearchProjects` is documented as substring search but currently uses exact equality, causing partial queries to fail in mock tests.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-stats.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-stats.ts:18">
P2: This effect can hammer `/api/v1/gitlab/stats` in a tight loop when the fetch fails (or returns null), because `stats` stays null and the guard never transitions to a loaded state.</violation>
</file>
<file name="apps/backend/internal/gitlab/poller.go">
<violation number="1" location="apps/backend/internal/gitlab/poller.go:38">
P1: Poller lifecycle state is not thread-safe; concurrent Start/Stop can race, leak loops, or block shutdown.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts:16">
P2: Action preset loading is tracked globally instead of per workspace, so one workspace request can block or misreport loading for another.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts:21">
P1: The effect can enter a continuous retry loop when the presets fetch fails, causing repeated requests and unnecessary load.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_sync.go">
<violation number="1" location="apps/backend/internal/gitlab/service_sync.go:15">
P2: TriggerMRSync breaks the optional-store contract by returning an error instead of an empty result when the store is unset.</violation>
</file>
<file name="apps/backend/cmd/kandev/main.go">
<violation number="1" location="apps/backend/cmd/kandev/main.go:443">
P2: GitLab service is started without wiring task cleanup dependencies, so GitLab cleanup operations can fail at runtime with "task deleter not configured".</violation>
</file>
<file name="apps/backend/internal/gitlab/handlers.go">
<violation number="1" location="apps/backend/internal/gitlab/handlers.go:121">
P2: Do not ignore payload parse errors in list/search handlers; malformed WS payloads currently fall through and can trigger unintended unfiltered operations.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts:33">
P1: The `loaded` guard is not workspace-scoped, so changing `workspaceId` can leave stale review watches and skip the needed refetch.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts:36">
P2: Guard async watch fetch callbacks by workspace so an older request cannot overwrite state after `workspaceId` changes.
(Based on your team's feedback about guarding workspace-scoped async fetches.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/action_presets_service.go">
<violation number="1" location="apps/backend/internal/gitlab/action_presets_service.go:11">
P2: `ResetActionPresets` should validate `workspaceID` to avoid silent resets with empty workspace IDs.</violation>
<violation number="2" location="apps/backend/internal/gitlab/action_presets_service.go:38">
P2: Partial updates incorrectly persist default presets for untouched kinds, which can freeze defaults and block future default updates.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts:27">
P1: `loaded` is not scoped per workspace, so changing `workspaceId` can skip refetch and leave stale issue watches from another workspace.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts:30">
P1: Guard the async callbacks so stale responses from a previous workspace cannot overwrite the current workspace state.
(Based on your team's feedback about guarding workspace-scoped async fetches.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/service_search.go">
<violation number="1" location="apps/backend/internal/gitlab/service_search.go:48">
P2: `GetStats` masks GitLab/auth/query failures as zero counts, which can return misleading stats instead of signaling an error.</violation>
</file>
<file name="apps/web/lib/api/domains/gitlab-api.ts">
<violation number="1" location="apps/web/lib/api/domains/gitlab-api.ts:146">
P2: `UpdateReviewWatchRequest` incorrectly allows `workspace_id`, but the backend update schema does not support it. This can lead callers to send unsupported fields that are silently ignored.</violation>
<violation number="2" location="apps/web/lib/api/domains/gitlab-api.ts:195">
P2: `UpdateIssueWatchRequest` should exclude `workspace_id`; allowing it in the type creates a frontend/backend contract mismatch.</violation>
</file>
<file name="apps/backend/internal/gitlab/store.go">
<violation number="1" location="apps/backend/internal/gitlab/store.go:86">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_review_watches` to avoid full scans in polling/list paths.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store.go:116">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_issue_watches` to keep watch polling queries scalable.</violation>
</file>
<file name="apps/backend/internal/gitlab/controller_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/controller_watches.go:173">
P2: Return 404 for missing review watches instead of mapping not-found to 500 in trigger endpoint.</violation>
<violation number="2" location="apps/backend/internal/gitlab/controller_watches.go:252">
P2: Return 404 for missing issue watches instead of treating not-found as internal server errors.</violation>
</file>
<file name="apps/backend/internal/gitlab/pat_client_actions.go">
<violation number="1" location="apps/backend/internal/gitlab/pat_client_actions.go:123">
P2: `ListUserProjects` only fetches the first page (`per_page=100`) and drops additional projects. This can silently hide repositories in project pickers/watch setup for users in many projects.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
26 issues found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/backend/internal/gitlab/store_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/store_watches.go:170">
P1: Normalize `PollIntervalSeconds` in `UpdateReviewWatch` the same way as create, otherwise updates can persist invalid (<=0) intervals.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store_watches.go:416">
P1: `UpdateIssueWatch` should clamp/default `PollIntervalSeconds`; currently invalid values can be persisted on update.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-status.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-status.ts:18">
P1: The initial-effect guard uses `status` truthiness instead of a loaded marker, so `null` results trigger repeated status refetches (disconnect/error loops).</violation>
</file>
<file name="apps/backend/internal/gitlab/service_cleanup.go">
<violation number="1" location="apps/backend/internal/gitlab/service_cleanup.go:59">
P1: Issue-watch fetch errors are defaulted to `CleanupPolicyAuto`, which can delete issue tasks incorrectly when watch metadata retrieval fails transiently. Erroring watches should be treated as unknown and skipped until a later sweep.
(Based on your team's feedback about treating watch fetch errors as unknown during cleanup sweeps.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/mock_client.go">
<violation number="1" location="apps/backend/internal/gitlab/mock_client.go:440">
P2: `SearchProjects` is documented as substring search but currently uses exact equality, causing partial queries to fail in mock tests.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-stats.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-stats.ts:18">
P2: This effect can hammer `/api/v1/gitlab/stats` in a tight loop when the fetch fails (or returns null), because `stats` stays null and the guard never transitions to a loaded state.</violation>
</file>
<file name="apps/backend/internal/gitlab/poller.go">
<violation number="1" location="apps/backend/internal/gitlab/poller.go:38">
P1: Poller lifecycle state is not thread-safe; concurrent Start/Stop can race, leak loops, or block shutdown.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts:16">
P2: Action preset loading is tracked globally instead of per workspace, so one workspace request can block or misreport loading for another.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts:21">
P1: The effect can enter a continuous retry loop when the presets fetch fails, causing repeated requests and unnecessary load.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_sync.go">
<violation number="1" location="apps/backend/internal/gitlab/service_sync.go:15">
P2: TriggerMRSync breaks the optional-store contract by returning an error instead of an empty result when the store is unset.</violation>
</file>
<file name="apps/backend/cmd/kandev/main.go">
<violation number="1" location="apps/backend/cmd/kandev/main.go:443">
P2: GitLab service is started without wiring task cleanup dependencies, so GitLab cleanup operations can fail at runtime with "task deleter not configured".</violation>
</file>
<file name="apps/backend/internal/gitlab/handlers.go">
<violation number="1" location="apps/backend/internal/gitlab/handlers.go:121">
P2: Do not ignore payload parse errors in list/search handlers; malformed WS payloads currently fall through and can trigger unintended unfiltered operations.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts:33">
P1: The `loaded` guard is not workspace-scoped, so changing `workspaceId` can leave stale review watches and skip the needed refetch.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts:36">
P2: Guard async watch fetch callbacks by workspace so an older request cannot overwrite state after `workspaceId` changes.
(Based on your team's feedback about guarding workspace-scoped async fetches.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/action_presets_service.go">
<violation number="1" location="apps/backend/internal/gitlab/action_presets_service.go:11">
P2: `ResetActionPresets` should validate `workspaceID` to avoid silent resets with empty workspace IDs.</violation>
<violation number="2" location="apps/backend/internal/gitlab/action_presets_service.go:38">
P2: Partial updates incorrectly persist default presets for untouched kinds, which can freeze defaults and block future default updates.</violation>
</file>
<file name="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts">
<violation number="1" location="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts:27">
P1: `loaded` is not scoped per workspace, so changing `workspaceId` can skip refetch and leave stale issue watches from another workspace.</violation>
<violation number="2" location="apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts:30">
P1: Guard the async callbacks so stale responses from a previous workspace cannot overwrite the current workspace state.
(Based on your team's feedback about guarding workspace-scoped async fetches.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/gitlab/service_search.go">
<violation number="1" location="apps/backend/internal/gitlab/service_search.go:48">
P2: `GetStats` masks GitLab/auth/query failures as zero counts, which can return misleading stats instead of signaling an error.</violation>
</file>
<file name="apps/web/lib/api/domains/gitlab-api.ts">
<violation number="1" location="apps/web/lib/api/domains/gitlab-api.ts:146">
P2: `UpdateReviewWatchRequest` incorrectly allows `workspace_id`, but the backend update schema does not support it. This can lead callers to send unsupported fields that are silently ignored.</violation>
<violation number="2" location="apps/web/lib/api/domains/gitlab-api.ts:195">
P2: `UpdateIssueWatchRequest` should exclude `workspace_id`; allowing it in the type creates a frontend/backend contract mismatch.</violation>
</file>
<file name="apps/backend/internal/gitlab/store.go">
<violation number="1" location="apps/backend/internal/gitlab/store.go:86">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_review_watches` to avoid full scans in polling/list paths.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store.go:116">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_issue_watches` to keep watch polling queries scalable.</violation>
</file>
<file name="apps/backend/internal/gitlab/controller_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/controller_watches.go:173">
P2: Return 404 for missing review watches instead of mapping not-found to 500 in trigger endpoint.</violation>
<violation number="2" location="apps/backend/internal/gitlab/controller_watches.go:252">
P2: Return 404 for missing issue watches instead of treating not-found as internal server errors.</violation>
</file>
<file name="apps/backend/internal/gitlab/pat_client_actions.go">
<violation number="1" location="apps/backend/internal/gitlab/pat_client_actions.go:123">
P2: `ListUserProjects` only fetches the first page (`per_page=100`) and drops additional projects. This can silently hide repositories in project pickers/watch setup for users in many projects.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| apps/backend/internal/gitlab/service_watches.go | MR/Review/Issue watch CRUD + polling logic; doc comment claims note detection that isn't implemented in CheckMRWatch, and dedup rows are orphaned on watch deletion. |
| apps/backend/internal/gitlab/service_issue_watches.go | Issue watch CRUD + polling; sweepIssueWatchTasksOnDelete leaves gitlab_issue_watch_tasks dedup rows orphaned after watch deletion. |
| apps/backend/internal/gitlab/poller.go | Background 3-loop poller with mutex-protected start/stop and proper WaitGroup tracking; clean implementation. |
| apps/backend/internal/gitlab/service_cleanup.go | Cleanup sweep for terminal MR/issue tasks with nil-store guards and cleanup-policy-aware logic; looks correct. |
| apps/backend/internal/orchestrator/event_handlers_gitlab.go | Event handlers for GitLab review/issue watch events; goroutines for task creation use context.Background() and are untracked (previously flagged). |
| apps/backend/internal/gitlab/store_watches.go | SQLite CRUD for all 5 new watch tables; dedup reservation uses INSERT+ON CONFLICT DO NOTHING pattern correctly; no per-watch bulk-delete helper exists for dedup rows. |
| apps/backend/internal/gitlab/service_merge.go | Merge, approve/unapprove, labels, assignees service layer with project-merge-method validation; clean. |
| apps/backend/internal/gitlab/handlers.go | ~35 WebSocket handlers for the full GitLab surface; consistent validation and error mapping patterns throughout. |
| apps/backend/internal/gitlab/controller_watches.go | HTTP watch/preset/write-action routes; consistent with WS surface; no auth middleware visible in diff but matches existing controller pattern. |
| apps/web/lib/state/slices/gitlab/gitlab-slice.ts | New state slice actions for review/issue/MR watches, presets, stats, and status; clean immer-based implementation. |
| apps/web/lib/state/store.ts | AppState type extended with new GitLab slice fields; state override boilerplate extracted to store-overrides.ts. |
Sequence Diagram
sequenceDiagram
participant Poller
participant Service
participant Store
participant GitLabAPI
participant EventBus
participant Orchestrator
participant TaskCreator
Poller->>Service: CheckReviewWatch(watch)
Service->>GitLabAPI: GetAuthenticatedUser
Service->>GitLabAPI: SearchMRs(filter, customQuery)
Service->>Store: HasReviewMRTask(watchID, project, iid)
Store-->>Service: exists?
Service->>Store: RecordReviewWatchPoll
Service->>EventBus: Publish(GitLabNewReviewMR, NewReviewMREvent)
EventBus->>Orchestrator: handleGitLabNewReviewMR
Orchestrator->>Service: ReserveReviewMRTask (dedup INSERT)
Orchestrator->>TaskCreator: CreateGitLabReviewTask
TaskCreator-->>Orchestrator: taskID
Orchestrator->>Service: AssignReviewMRTaskID
Reviews (14): Last reviewed commit: "fix(gitlab): address final review round ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Claude finished @jcfs's task in 6m 45s —— View job Code Review
SummaryThe latest commit (475bd15) addressed the two blockers from the previous Claude review: the label-filter bug in Remaining issues found in this pass: Blockers (must fix before merge)
Should fix (won't block CI but are real bugs)
Noted (intentional per PR description, no action required)
|
There was a problem hiding this comment.
5 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/backend/internal/gitlab/handlers.go">
<violation number="1" location="apps/backend/internal/gitlab/handlers.go:121">
P2: Do not ignore payload parse errors in list/search handlers; malformed WS payloads currently fall through and can trigger unintended unfiltered operations.</violation>
</file>
<file name="apps/backend/internal/gitlab/action_presets_service.go">
<violation number="1" location="apps/backend/internal/gitlab/action_presets_service.go:11">
P2: `ResetActionPresets` should validate `workspaceID` to avoid silent resets with empty workspace IDs.</violation>
<violation number="2" location="apps/backend/internal/gitlab/action_presets_service.go:38">
P2: Partial updates incorrectly persist default presets for untouched kinds, which can freeze defaults and block future default updates.</violation>
</file>
<file name="apps/web/lib/api/domains/gitlab-api.ts">
<violation number="1" location="apps/web/lib/api/domains/gitlab-api.ts:146">
P2: `UpdateReviewWatchRequest` incorrectly allows `workspace_id`, but the backend update schema does not support it. This can lead callers to send unsupported fields that are silently ignored.</violation>
<violation number="2" location="apps/web/lib/api/domains/gitlab-api.ts:195">
P2: `UpdateIssueWatchRequest` should exclude `workspace_id`; allowing it in the type creates a frontend/backend contract mismatch.</violation>
</file>
<file name="apps/backend/internal/gitlab/store.go">
<violation number="1" location="apps/backend/internal/gitlab/store.go:86">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_review_watches` to avoid full scans in polling/list paths.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store.go:116">
P2: Add indexes for `workspace_id` and `enabled` on `gitlab_issue_watches` to keep watch polling queries scalable.</violation>
</file>
<file name="apps/backend/internal/gitlab/controller_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/controller_watches.go:173">
P2: Return 404 for missing review watches instead of mapping not-found to 500 in trigger endpoint.</violation>
<violation number="2" location="apps/backend/internal/gitlab/controller_watches.go:252">
P2: Return 404 for missing issue watches instead of treating not-found as internal server errors.</violation>
</file>
<file name="apps/backend/internal/gitlab/store_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/store_watches.go:170">
P1: Normalize `PollIntervalSeconds` in `UpdateReviewWatch` the same way as create, otherwise updates can persist invalid (<=0) intervals.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store_watches.go:416">
P1: `UpdateIssueWatch` should clamp/default `PollIntervalSeconds`; currently invalid values can be persisted on update.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_search.go">
<violation number="1" location="apps/backend/internal/gitlab/service_search.go:48">
P2: `GetStats` masks GitLab/auth/query failures as zero counts, which can return misleading stats instead of signaling an error.</violation>
</file>
<file name="apps/backend/internal/gitlab/mock_client.go">
<violation number="1" location="apps/backend/internal/gitlab/mock_client.go:440">
P2: `SearchProjects` is documented as substring search but currently uses exact equality, causing partial queries to fail in mock tests.</violation>
</file>
<file name="apps/backend/internal/gitlab/poller.go">
<violation number="1" location="apps/backend/internal/gitlab/poller.go:38">
P1: Poller lifecycle state is not thread-safe; concurrent Start/Stop can race, leak loops, or block shutdown.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_sync.go">
<violation number="1" location="apps/backend/internal/gitlab/service_sync.go:15">
P2: TriggerMRSync breaks the optional-store contract by returning an error instead of an empty result when the store is unset.</violation>
</file>
<file name="apps/backend/internal/gitlab/pat_client_actions.go">
<violation number="1" location="apps/backend/internal/gitlab/pat_client_actions.go:123">
P2: `ListUserProjects` only fetches the first page (`per_page=100`) and drops additional projects. This can silently hide repositories in project pickers/watch setup for users in many projects.</violation>
</file>
<file name="apps/backend/internal/gitlab/store_watches_test.go">
<violation number="1" location="apps/backend/internal/gitlab/store_watches_test.go:78">
P2: The post-delete assertion drops `GetReviewWatch` errors, which can let the test pass on query failures.</violation>
<violation number="2" location="apps/backend/internal/gitlab/store_watches_test.go:91">
P2: The test ignores an error on the duplicate reserve path, so it can pass even when the operation fails unexpectedly.</violation>
</file>
<file name="apps/backend/internal/gitlab/service_watches.go">
<violation number="1" location="apps/backend/internal/gitlab/service_watches.go:786">
P2: `appendLabelsToQuery` uses a raw substring match for `labels=`; parse query params instead to avoid skipping label injection on false matches.</violation>
</file>
<file name="apps/backend/internal/orchestrator/event_handlers_gitlab.go">
<violation number="1" location="apps/backend/internal/orchestrator/event_handlers_gitlab.go:82">
P2: This adds a new ad-hoc watcher task-creation flow instead of using `WatcherDispatchCoordinator`, reintroducing the `createXIssueTask` pattern that the backend architecture explicitly forbids.</violation>
<violation number="2" location="apps/backend/internal/orchestrator/event_handlers_gitlab.go:83">
P2: Returning early when `gitlabService` is nil drops watch events; this should fail open (skip dedup) instead of skipping task creation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/internal/gitlab/handlers.go`:
- Around line 553-563: The wsNewDiscussionNote WebSocket handler currently
doesn't validate that the incoming request Body is non-empty, letting empty
discussion notes proceed and cause internal errors; update the handler
(wsNewDiscussionNote) to check req.Body after parseInto and return
badRequest(msg, "project, iid, discussion_id, body required") when req.Body ==
"" (alongside existing checks for Project, IID, and DiscussionID) so empty-body
requests are rejected like the HTTP handler.
In `@apps/backend/internal/gitlab/mock_client.go`:
- Around line 433-444: The mock SearchProjects implementation is too strict
because MockClient.SearchProjects only returns projects when p.PathWithNamespace
or p.Path exactly equals the query; change it to perform partial (substring)
matching so autocomplete/search flows work: normalize the query (trim and
optionally lowercase) and check if strings.Contains(p.PathWithNamespace, query)
|| strings.Contains(p.Path, query) (use case-insensitive comparison if
production does) and append matches to out; keep the same return types and
behavior when query is empty.
In `@apps/backend/internal/gitlab/service_cleanup.go`:
- Around line 147-151: The current check calls
checker.HasUserAuthoredMessage(ctx, t.TaskID) but ignores the error, which can
cause deletion on failure; change the logic in the block that checks
CleanupPolicyAuto to capture the (authored, err) result and if err != nil treat
it as a fail-safe by returning false (i.e., do not proceed with cleanup). Update
the conditional around HasUserAuthoredMessage (referencing checker,
HasUserAuthoredMessage, CleanupPolicyAuto, and t.TaskID) so any error causes an
immediate false return instead of continuing.
- Around line 74-78: The check currently discards the error from
checker.HasUserAuthoredMessage(ctx, t.TaskID) causing a fail-open delete; change
the logic in the CleanupPolicyAuto branch to handle the error explicitly: call
checker.HasUserAuthoredMessage(ctx, t.TaskID), if it returns (true, nil) return
false (preserve), if it returns (false, err) treat this as a safe failure — log
the error and return false to skip deletion (i.e., preserve the task) so
transient errors don't cause user-interacted tasks to be deleted; update the
code around CleanupPolicyAuto and the checker.HasUserAuthoredMessage call
accordingly.
In `@apps/backend/internal/gitlab/service_watches.go`:
- Around line 233-235: Normalize the updated poll interval the same way the
create path does instead of writing raw values: in the update handler where
req.PollIntervalSeconds is checked and assigned to rw.PollIntervalSeconds, read
the incoming value into a local (e.g., v := *req.PollIntervalSeconds) and clamp
it using the same bounds used by create (e.g., if v <= 0 then set to
DefaultPollIntervalSeconds, and if v < MinPollIntervalSeconds set to
MinPollIntervalSeconds, and optionally cap to MaxPollIntervalSeconds if
applicable) before assigning rw.PollIntervalSeconds = v; use the same constants
or helper (MinPollIntervalSeconds, DefaultPollIntervalSeconds or the create-path
clamp function) to ensure consistent behavior.
- Around line 457-470: UpdateIssueWatch currently calls applyIssueWatchPatch(iw,
req) without checking if req is nil, causing a panic when req is nil; add a nil
guard: only call applyIssueWatchPatch when req != nil, and only validate
req.CleanupPolicy when req is non-nil (i.e., move the CleanupPolicy validation
inside the req != nil branch). Locate UpdateIssueWatch and implement the
nil-check around applyIssueWatchPatch and the IsValidCleanupPolicy check so the
function safely updates the issue watch when req is provided and simply persists
the existing iw when req is nil.
- Around line 190-203: UpdateReviewWatch currently calls
applyReviewWatchPatch(rw, req) and dereferences req.CleanupPolicy without
guarding req for nil; add a nil-check at the start of UpdateReviewWatch to
return a clear error (e.g., "nil update request") if req is nil, and only call
applyReviewWatchPatch and evaluate req.CleanupPolicy when req != nil. Reference:
UpdateReviewWatch, applyReviewWatchPatch, IsValidCleanupPolicy,
requireStore().GetReviewWatch/UpdateReviewWatch.
- Around line 786-787: The current check uses strings.Contains(customQuery,
"labels=") which can false-match keys like "mylabels="; update the condition to
detect the literal query key "labels" instead—e.g. parse the query with net/url
(url.ParseQuery(strings.TrimPrefix(customQuery, "?"))) and test for the "labels"
key in the returned map, or use a regex that looks for "(^|&|\\?)labels=";
replace the existing strings.Contains(customQuery, "labels=") usage accordingly
so existence is determined by the actual query key, not a raw substring.
In `@apps/backend/internal/orchestrator/event_handlers_gitlab.go`:
- Line 98: The calls to s.gitlabService.ReleaseReviewMRTask (and the similar
ReleaseReviewIssueTask) currently ignore returned errors; change those call
sites to capture the error, log a clear error message including identifiers
(evt.ReviewWatchID, mr.ProjectPath, mr.IID or issue equivalents) and return or
surface the error so the caller can handle retries/cleanup rather than silently
leaving the dedup-reservation in place; ensure you update both the
ReleaseReviewMRTask and ReleaseReviewIssueTask invocations to check the error
and handle it consistently.
In `@apps/web/hooks/domains/gitlab/use-gitlab-action-presets.ts`:
- Around line 20-29: The effect currently swallows errors and never marks
presets as attempted, causing an infinite refetch when workspaceId exists and
presets is still unset; update the .catch handler for
getActionPresets(workspaceId) to set a sentinel value via the existing setter
(e.g., call set(workspaceId, null) or set(workspaceId, [])) to mark that the
fetch was attempted and failed, and keep the .finally(() => setLoading(false))
as-is so loading is cleared; reference the useEffect block and the
functions/variables getActionPresets, set, presets, loading, workspaceId, and
setLoading to locate where to add the catch-side set call.
In `@apps/web/hooks/domains/gitlab/use-gitlab-issue-watches.ts`:
- Around line 26-33: The effect uses a global loaded/loading flag so switching
workspaceId can skip fetching and show stale watches; make the load state
scope-aware by tracking which workspace was last loaded (e.g., add
lastLoadedWorkspaceId or a map keyed by workspaceId) and update setLoading/set
accordingly: when workspaceId changes clear or compare against
lastLoadedWorkspaceId before returning early, and ensure the useEffect depends
on workspaceId and lastLoadedWorkspaceId so listIssueWatches(workspaceId, ...)
runs whenever the current workspace’s watches haven’t been loaded yet; update
the logic around useEffect, loaded, loading, listIssueWatches, setLoading, and
set to reference the per-workspace loaded marker.
In `@apps/web/hooks/domains/gitlab/use-gitlab-review-watches.ts`:
- Around line 32-39: The effect currently gates fetching by a global loaded flag
so when workspaceId changes and loaded is already true the fetch is skipped
leaving stale data; update the logic in the useEffect that references
workspaceId, loaded, loading, listReviewWatches, set and setLoading so that
loaded is scoped to the current workspace (e.g., track loadedForWorkspaceId or
reset loaded when workspaceId changes) and/or include workspaceId in the gating
check—ensure the effect triggers a fetch for a new workspaceId by clearing or
scoping the loaded flag before calling listReviewWatches and then set or
setLoading as before.
In `@apps/web/hooks/domains/gitlab/use-gitlab-stats.ts`:
- Around line 17-24: The effect currently keeps retrying because a failed fetch
sets stats to null (falsy) so the guard (if (stats || loading) return) doesn't
stop subsequent runs; add a failure flag (e.g., statsError with setStatsError)
or a retry counter and set it in the catch of fetchGitLabStats, then include
that flag/counter in the effect's guard and dependency array so the effect
returns when a permanent failure is detected; update the catch handler to call
setStatsError(true) (or increment the retry counter) instead of only
setStats(null), and add statsError (or retry count) to the if-check and
dependency list to prevent endless retries.
In `@apps/web/hooks/domains/gitlab/use-gitlab-status.ts`:
- Around line 17-24: The effect in use-gitlab-status (the useEffect that checks
status/loading and calls fetchGitLabStatus) re-triggers after a failed fetch
because status is set to null; prevent the retry loop by adding and checking a
fetch-attempt flag (e.g., statusTried or statusFetchError) in the hook state and
dependencies, set that flag to true in both .then and .catch handlers (or set an
error flag in .catch), and update the effect condition to return early if
statusTried (or statusFetchError) is true; use the existing symbols status,
loading, setStatus, setStatusLoading, and fetchGitLabStatus to implement this
change so a failed fetch does not immediately re-run.
In `@apps/web/lib/state/slices/gitlab/gitlab-slice.ts`:
- Around line 6-8: The slice's workspace-scoped watch state
(gitlabReviewWatches, gitlabIssueWatches, gitlabMRWatches) uses a single global
loaded/loading/items bag which can serve stale data across workspace switches;
change the shape to key items/loading/loaded by workspaceId (e.g. {
[workspaceId]: { items, loaded, loading } }) or add a currentLoadedWorkspaceId
per watch and invalidate/reset the loaded flag and items when workspaceId
changes, updating all selectors and reducers that read/write gitlab*Watches (and
the corresponding logic referenced in the same file lines ~58-117) so consumers
always fetch fresh lists for the active workspace.
In `@apps/web/lib/state/store.ts`:
- Around line 190-195: Post-slice override only restores taskMRs but misses the
new GitLab fields so merged/hydrated values for gitlabReviewWatches,
gitlabIssueWatches, gitlabMRWatches, gitlabActionPresets, gitlabStats and
gitlabStatus get lost; update the post-slice override in store.ts (the block
that restores from merged state after slice creation) to also copy these
properties from the merged/hydrated state into the store (same pattern used for
taskMRs) so the new GitLab sub-state is preserved on store creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e05f6286-65df-42cd-a3e6-54113d4a9926
📒 Files selected for processing (40)
apps/backend/cmd/kandev/helpers.goapps/backend/cmd/kandev/main.goapps/backend/cmd/kandev/services.goapps/backend/internal/events/types.goapps/backend/internal/gitlab/action_presets_service.goapps/backend/internal/gitlab/action_presets_test.goapps/backend/internal/gitlab/client.goapps/backend/internal/gitlab/controller.goapps/backend/internal/gitlab/controller_watches.goapps/backend/internal/gitlab/handlers.goapps/backend/internal/gitlab/mock_client.goapps/backend/internal/gitlab/mock_controller.goapps/backend/internal/gitlab/noop_client.goapps/backend/internal/gitlab/pat_client_actions.goapps/backend/internal/gitlab/poller.goapps/backend/internal/gitlab/service.goapps/backend/internal/gitlab/service_cleanup.goapps/backend/internal/gitlab/service_merge.goapps/backend/internal/gitlab/service_search.goapps/backend/internal/gitlab/service_sync.goapps/backend/internal/gitlab/service_watches.goapps/backend/internal/gitlab/service_watches_test.goapps/backend/internal/gitlab/store.goapps/backend/internal/gitlab/store_watches.goapps/backend/internal/gitlab/store_watches_test.goapps/backend/internal/gitlab/watch_models.goapps/backend/internal/orchestrator/event_handlers_gitlab.goapps/backend/internal/orchestrator/service.goapps/backend/pkg/websocket/actions.goapps/web/hooks/domains/gitlab/use-gitlab-action-presets.tsapps/web/hooks/domains/gitlab/use-gitlab-issue-watches.tsapps/web/hooks/domains/gitlab/use-gitlab-review-watches.tsapps/web/hooks/domains/gitlab/use-gitlab-stats.tsapps/web/hooks/domains/gitlab/use-gitlab-status.tsapps/web/lib/api/domains/gitlab-api.tsapps/web/lib/state/slices/gitlab/gitlab-slice.test.tsapps/web/lib/state/slices/gitlab/gitlab-slice.tsapps/web/lib/state/slices/gitlab/types.tsapps/web/lib/state/store.tsapps/web/lib/types/gitlab.ts
|
Claude finished @jcfs's task in 5m 45s —— View job Code Review
FindingsBlocker (must fix before merge)1. 2. 3. Poll interval not normalized in update path — Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — fix blockers first. |
|
Claude finished @jcfs's task in 3m 16s —— View job Code Review
FindingsAll previously flagged blockers are confirmed resolved in commit
Suggestion (recommended, doesn't block)1. Initial-check goroutines use
2. Test assertions silently drop errors — // line 78 — GetReviewWatch error ignored after delete
got3, _ := store.GetReviewWatch(ctx, rw.ID)
// line 91 — second Reserve error ignored
ok2, _ := store.ReserveReviewMRTask(ctx, "watch-1", "group/project", 42, "https://x")The tests check the Summary
Verdict: Ready to merge. All blockers from prior rounds are resolved. The two suggestions are carry-overs (items 1 and 2 were flagged in earlier rounds as suggestions); they don't affect correctness or production safety. |
| if err := store.CreateReviewWatch(ctx, rw); err != nil { | ||
| return nil, fmt.Errorf("create review watch: %w", err) | ||
| } | ||
| go s.initialReviewCheck(context.Background(), rw) |
There was a problem hiding this comment.
Suggestion (carry-over from prior rounds): goroutine uses context.Background() — uncancellable on shutdown
initialReviewCheck is launched with context.Background() and is not tracked by any WaitGroup. After Poller.Stop() returns the goroutine may still be running and publishing events to a draining bus. The same pattern applies to initialIssueCheck in service_issue_watches.go:42.
Low production risk (the check is short-lived), but non-idiomatic and would trip goleak.VerifyTestMain if the package ever adds goroutine-leak testing (which internal/gitlab doesn't have yet — see AGENTS.md on which packages are instrumented).
Suggested change
| go s.initialReviewCheck(context.Background(), rw) | |
| go s.initialReviewCheck(ctx, rw) |
Passing the caller's ctx here (which is HTTP-request-scoped) bounds the goroutine to the request lifetime. A longer-lived service context (set at Service.Start time) would be even better for a recurring background goroutine, but either is better than Background().
| if err := store.DeleteReviewWatch(ctx, rw.ID); err != nil { | ||
| t.Fatalf("DeleteReviewWatch: %v", err) | ||
| } | ||
| got3, _ := store.GetReviewWatch(ctx, rw.ID) |
There was a problem hiding this comment.
Suggestion: error silently dropped — test can pass on unrelated DB failure
The blank identifier drops the error return. If GetReviewWatch fails for any reason other than "not found" (e.g., schema issue), the test still passes because got3 would be nil and the assertion if got3 != nil would succeed.
Suggested change
| got3, _ := store.GetReviewWatch(ctx, rw.ID) | |
| got3, err3 := store.GetReviewWatch(ctx, rw.ID) | |
| if err3 != nil { | |
| t.Fatalf("GetReviewWatch after delete: %v", err3) | |
| } |
Same pattern at line 71 (got2, _ := ...).
Comment on lines
+220
to
+242
| func (s *Service) fetchIssues(ctx context.Context, watch *IssueWatch) ([]*Issue, error) { | ||
| client := s.Client() | ||
| username, err := client.GetAuthenticatedUser(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("resolve gitlab username: %w", err) | ||
| } | ||
| // When a custom_query is set, labels need to be folded into it (the | ||
| // client's buildIssueSearchQuery returns customQuery verbatim and | ||
| // ignores the auxiliary `filter` arg). Otherwise build a default | ||
| // "assigned to me" filter and append labels there. | ||
| filter := "" | ||
| customQuery := watch.CustomQuery | ||
| switch { | ||
| case customQuery != "": | ||
| if len(watch.Labels) > 0 { | ||
| customQuery = appendLabelsToQuery(customQuery, watch.Labels) | ||
| } | ||
| default: | ||
| filter = "assignee_username=" + url.QueryEscape(username) | ||
| if len(watch.Labels) > 0 { | ||
| filter += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ",")) | ||
| } | ||
| } |
There was a problem hiding this comment.
GetAuthenticatedUser inside the branch that actually needs it. When customQuery != "", username is never referenced, so calling the user-info endpoint unconditionally wastes a round-trip and makes custom-query watches fail if that endpoint is unavailable or the PAT lacks read_user scope.
Suggested change
| func (s *Service) fetchIssues(ctx context.Context, watch *IssueWatch) ([]*Issue, error) { | |
| client := s.Client() | |
| username, err := client.GetAuthenticatedUser(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("resolve gitlab username: %w", err) | |
| } | |
| // When a custom_query is set, labels need to be folded into it (the | |
| // client's buildIssueSearchQuery returns customQuery verbatim and | |
| // ignores the auxiliary `filter` arg). Otherwise build a default | |
| // "assigned to me" filter and append labels there. | |
| filter := "" | |
| customQuery := watch.CustomQuery | |
| switch { | |
| case customQuery != "": | |
| if len(watch.Labels) > 0 { | |
| customQuery = appendLabelsToQuery(customQuery, watch.Labels) | |
| } | |
| default: | |
| filter = "assignee_username=" + url.QueryEscape(username) | |
| if len(watch.Labels) > 0 { | |
| filter += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ",")) | |
| } | |
| } | |
| func (s *Service) fetchIssues(ctx context.Context, watch *IssueWatch) ([]*Issue, error) { | |
| client := s.Client() | |
| // When a custom_query is set, labels need to be folded into it (the | |
| // client's buildIssueSearchQuery returns customQuery verbatim and | |
| // ignores the auxiliary `filter` arg). Otherwise build a default | |
| // "assigned to me" filter and append labels there. | |
| filter := "" | |
| customQuery := watch.CustomQuery | |
| switch { | |
| case customQuery != "": | |
| if len(watch.Labels) > 0 { | |
| customQuery = appendLabelsToQuery(customQuery, watch.Labels) | |
| } | |
| default: | |
| username, err := client.GetAuthenticatedUser(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("resolve gitlab username: %w", err) | |
| } | |
| filter = "assignee_username=" + url.QueryEscape(username) | |
| if len(watch.Labels) > 0 { | |
| filter += "&labels=" + url.QueryEscape(strings.Join(watch.Labels, ",")) | |
| } | |
| } |
jcfs
force-pushed
the
feature/full-gitlab-integrat-5h2
branch
from
49cd445 to
b92ae5b
Compare
|
Claude finished @jcfs's task in 6m 15s —— View job Code Review — PR #1120: Full GitLab IntegrationRound 4 · Final pass · All blockers resolved ✅ SummaryAll blockers raised in previous review rounds have been addressed in commits FindingsSuggestions (non-blocking)S1 —
go s.initialReviewCheck(context.Background(), rw)
go s.initialIssueCheck(context.Background(), iw)These goroutines run at watch-creation time to perform an immediate first check. Because they use S2 — Dropped errors in test assertions (carry-over from all prior rounds)
got2, _ := store.GetReviewWatch(ctx, rw.ID) // line 71
got3, _ := store.GetReviewWatch(ctx, rw.ID) // line 78
ok2, _ := store.ReserveReviewMRTask(...) // line 91Dropping errors means a storage layer regression (e.g. the row not being deleted) would silently produce a wrong assertion value rather than a clear S3 — Missing
Minimal addition: // apps/backend/internal/gitlab/goleak_test.go
package gitlab_test
import (
"testing"
"go.uber.org/goleak"
)
func TestMain(m *testing.M) {
goleak.VerifyTestMain(m)
}Resolved since last round ✅All items from rounds 1–3 are confirmed resolved:
VerdictReady to merge. The three suggestions above are clean-up items that can be addressed in follow-up PRs without blocking the integration. The dedup-reservation pipeline, poller lifecycle, nil-store guards, cleanup policies, and frontend state management are all correctly implemented. |
Comment on lines
+400
to
+418
| func (s *Service) fetchReviewMRs(ctx context.Context, watch *ReviewWatch) ([]*MR, error) { | ||
| client := s.Client() | ||
| username, err := client.GetAuthenticatedUser(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("resolve gitlab username: %w", err) | ||
| } | ||
| if username == "" { | ||
| return nil, fmt.Errorf("no authenticated gitlab user") | ||
| } | ||
| // SearchMRs's buildMRSearchQuery returns customQuery verbatim when | ||
| // non-empty (ignoring filter), so only build the default filter when | ||
| // the watch has no customQuery to pass through. | ||
| filter := "" | ||
| if watch.CustomQuery == "" { | ||
| filter = "reviewer_username=" + url.QueryEscape(username) | ||
| } | ||
| mrs, err := client.SearchMRs(ctx, filter, watch.CustomQuery) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("search MRs: %w", err) |
There was a problem hiding this comment.
GetAuthenticatedUser called unconditionally in fetchReviewMRs
username is only referenced inside the if watch.CustomQuery == "" branch, but client.GetAuthenticatedUser is called before that guard. For any review watch that has a CustomQuery set, the read_user API round-trip is wasted every poll cycle, and — identically to the already-flagged pattern in fetchIssues — the whole poll will error out if the PAT's scope doesn't include read_user. Move the call inside the guard so custom-query watches don't depend on a permission they don't need.
| if err := store.CreateReviewWatch(ctx, rw); err != nil { | ||
| return nil, fmt.Errorf("create review watch: %w", err) | ||
| } | ||
| go s.initialReviewCheck(context.Background(), rw) |
There was a problem hiding this comment.
S1 (carry-over): This goroutine uses context.Background(), so it won't be cancelled when the poller stops. The window is small (one HTTP round-trip), but it's inconsistent with the rest of the package. Consider holding a cancellable root context on Service (set at Start, cancelled at Stop) and passing it here instead.
| if err := store.CreateIssueWatch(ctx, iw); err != nil { | ||
| return nil, fmt.Errorf("create issue watch: %w", err) | ||
| } | ||
| go s.initialIssueCheck(context.Background(), iw) |
There was a problem hiding this comment.
S1 (carry-over): Same context.Background() issue as initialReviewCheck in service_watches.go:199 — won't be cancelled on poller stop. Same fix applies: pass a service-level cancellable context.
| if err := store.UpdateReviewWatch(ctx, rw); err != nil { | ||
| t.Fatalf("UpdateReviewWatch: %v", err) | ||
| } | ||
| got2, _ := store.GetReviewWatch(ctx, rw.ID) |
There was a problem hiding this comment.
S2 (carry-over): Errors dropped with _ on lines 71, 78, and 91. A storage regression (row not deleted, reserve not rejected) would silently produce a wrong assertion rather than a clear failure. Use require.NoError(t, err) for each.
5 tasks
Add the watcher/poll/write-action surface on top of the existing GitLab client + service skeleton: * 5 new SQLite tables (mr_watches, review_watches, review_mr_tasks, issue_watches, issue_watch_tasks, action_presets) + CRUD * Client + Service: MergeMR, GetProjectMergeMethods, GetProtectedBranch, ListUserProjects, SearchProjects, SetMRLabels, SetMRAssignees * Service: review/issue/MR watch CRUD + Check/Trigger, GetStats, CleanupAllReview/IssueTasks (cleanup-policy aware), TriggerMRSync, action-presets CRUD with defaults * Background poller (MR / review / issue loops) wired from main.go * ~35 GitLab WS action constants + handlers.go registrations * Mock controller for E2E seeding (MRs, issues, pipelines, discussions, approvals, branches) * eventBus + taskDeleter + taskSessionChecker plumbed through Service * Event types: GitLabMRFeedback / NewReviewMR / NewIssue / TaskMRUpdated
…ooks Extend the frontend GitLab integration on top of the new backend surface: * HTTP API client expansion (gitlab-api.ts): full CRUD for review/issue/MR watches, MR write actions (merge/approve/labels/assignees), MR files/commits/feedback, action presets, projects autocomplete, stats * Store slice expansion: review/issue/MR watches lists, action presets by workspace, stats + status with loading flags. Properties prefixed with gitlab to avoid collision with GitHub slice * Domain hooks: use-gitlab-status, use-gitlab-stats, use-gitlab-review-watches, use-gitlab-issue-watches, use-gitlab-action-presets — mirror github hook shapes * Type model: ReviewWatch, IssueWatch, MRWatch, ActionPresets, Stats, ProjectMergeMethods, MRApproval, Pipeline, MRFeedback, etc. * Backend controller HTTP routes for watches/presets/write-actions/ projects/stats so the frontend HTTP API client has endpoints to hit All passes: backend (build + vet + lint + tests), web (typecheck + lint + 2400 tests).
… add tests Address Claude review blockers on the parity PR: 1. Wire orchestrator subscriptions for GitLab review/issue watch events. Add event_handlers_gitlab.go with handleGitLabNewReviewMR / handleGitLabNewIssue, dedup reservation handshake, and task-creator interfaces (mirrors event_handlers_github.go). main.go now calls orchestratorSvc.SetGitLabService so the dedup APIs are usable 2. Fix silent label-filter drop: when a custom_query is set on an issue watch, labels were being appended to the unused filter arg. Fold labels into customQuery (or default filter) so they actually reach GitLab; extracted appendLabelsToQuery helper with explicit handling of pre-existing labels= clauses 3. Switch event payloads to pointer types (mirrors GitHub) so handlers can type-assert pointer events 4. Backend tests: store_watches_test.go (CRUD + reserve/assign for all 4 new tables + presets), service_watches_test.go (label-filter helper, cleanup policy, project normalisation), action_presets_test.go 5. Frontend slice tests: review/issue watch CRUD round-trips, action presets + stats reducer coverage All green: backend (build+vet+lint+tests), web (typecheck+lint+2403 tests).
…ts, nil guards Address the second round of Claude review blockers: 1. main.go: wire SetTaskDeleter + SetTaskSessionChecker on services.GitLab (mirrors GitHub). Without these, manual cleanup sweeps would always error with "task deleter not configured" 2. service_cleanup.go: default cleanup policy on transient DB error is now CleanupPolicyNever (preserve tasks) instead of CleanupPolicyAuto (silently delete). Genuine "watch was deleted" path also falls under the safe-side default; user can manually delete the orphan tasks 3. service_watches.go: add nil guards on requireStore() so the 6 MR-watch list/get/delete methods return an errStoreUnavailable error instead of panicking when the SQLite store failed to construct at boot 4. use-gitlab-status.ts + use-gitlab-stats.ts: add per-mount attemptedRef so an unreachable GitLab doesn't trigger an infinite re-fetch loop — the previous useEffect re-ran every render because the failure path left status null, satisfying the !status guard
…pdate Address remaining Claude blockers: * service_cleanup.go: HasUserAuthoredMessage transient errors are now preserved (return false → skip delete) instead of silently ignored; the alternative was occasionally deleting tasks a user had touched * service_watches.go: extract clampPollInterval helper applying the same bounds (0 → default, <60 → 60) as the create path; both applyReviewWatchPatch and applyIssueWatchPatch use it so user-supplied zero or tiny values via UpdateXxxWatchRequest no longer hammer GitLab
…atch cache * service_watches.go: applyReviewWatchPatch / applyIssueWatchPatch get nolint:dupl markers — they share shape but per-domain field validation lives in the create paths, so deduplicating via generics would obscure the contract. Restores backend lint to 0 issues * use-gitlab-action-presets.ts: per-workspace attemptedRef set so a failing preset fetch doesn't retry on every render * use-gitlab-review-watches.ts + use-gitlab-issue-watches.ts: track lastFetchedRef per consumer so a workspace switch triggers a refetch. The slice-level loaded flag is shared across instances and can't double as a per-workspace cache key
… paths Round-3 Claude finding: ReviewWatch / IssueWatch / preset / reservation methods still called s.requireStore().X() directly. If NewStore fails at boot (table migration error), the service struct's store is nil and every list/get/update/create/reserve method panics on first request. Apply the same store-nil → errStoreUnavailable pattern from f95a929 to every remaining method: * CreateReviewWatch / CreateIssueWatch * GetReviewWatch / ListReviewWatches / ListAllReviewWatches / UpdateReviewWatch / TriggerReviewWatchAll * GetIssueWatch / ListIssueWatches / ListAllIssueWatches / UpdateIssueWatch / TriggerIssueWatchAll * ReserveReviewMRTask / AssignReviewMRTaskID / ReleaseReviewMRTask * ReserveIssueWatchTask / AssignIssueWatchTaskID / ReleaseIssueWatchTask * lookupReviewPolicy / lookupIssuePolicy (return CleanupPolicyNever)
…anup paths Round-4 Claude finding: 4 remaining panic sites the previous nil-guard pass missed — these run from the background poller (every 5 min) and the manual delete/cleanup flows, so a boot-time NewStore failure would crash the orchestrator process on first tick. * service_watches.go: add store-nil guards to DeleteReviewWatch / DeleteIssueWatch / CheckReviewWatch / CheckIssueWatch * service_cleanup.go: guard CleanupAllReviewTasks / CleanupAllIssueTasks entry points; wrap the trailing DeleteReviewMRTask / DeleteIssueWatchTask calls in `if store := s.requireStore(); store != nil` blocks so the cleanup succeeds even when the dedup-row delete is unreachable
…ive limit CI's --new-from-rev lint caught service_watches.go at 808 lines (limit 800). Split into three focused files: * service_events.go (94 LOC) — publish helpers for MR feedback / new review MR / new issue / watch lifecycle events * service_issue_watches.go (302 LOC) — Issue watch CRUD + Check + Trigger + fetch + helpers * service_reservations.go (59 LOC) — Reserve/Assign/Release dedup handles used by the orchestrator event handlers service_watches.go is now 505 lines and contains MR watch + Review watch only. No behavioral change.
Address actionable inline comments from Claude/CodeRabbit/cubic/Greptile: * appendLabelsToQuery: use url.ParseQuery for exact key match instead of strings.Contains; previously false-matched keys like mylabels= and silently dropped the watch's labels (test added) * fetchReviewMRs: drop dead `filter = watch.CustomQuery` assignment — SearchMRs's buildMRSearchQuery returns customQuery verbatim and ignores filter when customQuery is non-empty * store.go: add workspace_id indexes on gitlab_review_watches and gitlab_issue_watches (the 5-min poller and HTTP list endpoints did full table scans) * mock_client.SearchProjects: switch from == to case-insensitive Contains so partial-query autocomplete returns the seeded project, matching the doc comment * Poller: add sync.Mutex to guard `started` field; Start/Stop are now safe to call concurrently (go test -race would have flagged the previous read/write race) * Add ErrWatchNotFound sentinel; controller_watches.go maps it to HTTP 404 via httpRespondError helper. Update/Trigger handlers previously returned 500 for missing watches, hiding the distinction from real server faults * service_events.go: log Publish errors on watch lifecycle events instead of swallowing them silently * handlers.go: reject malformed JSON payloads in list-style WS handlers rather than silently falling through to broader list/search behavior * event_handlers_gitlab.go: remove dead gitlabRepoSlug helper + blank-identifier suppression
…gs, body validation - CheckMRWatch, UpdateReviewWatch, UpdateIssueWatch: nil guards for store/req - handlers: wsNewDiscussionNote rejects empty body - action_presets: Update reads raw stored row (no default freezing); Reset validates workspace_id - service_search.GetStats: log Warn per sub-call failure before zero - event_handlers_gitlab: log Release* dedup-row errors with context - web/store.ts: post-slice merge restores every GitLab sub-state field; extract buildStateOverrides helper - web/default-state.ts: add GitLab fields to defaultState + mergeInitialState via mergeGitLabFields helper - web/gitlab-api.ts: UpdateXxxWatchRequest now Omit workspace_id Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jcfs
force-pushed
the
feature/full-gitlab-integrat-5h2
branch
from
b92ae5b to
36c9bd6
Compare
|
Claude finished @jcfs's task in 6m 13s —— View job Code Review — PR #1120: full GitLab integrationReview complete. All prior blockers from earlier rounds are resolved. One new blocker identified. FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — add |
| if err := store.UpdateReviewWatch(ctx, rw); err != nil { | ||
| t.Fatalf("UpdateReviewWatch: %v", err) | ||
| } | ||
| got2, _ := store.GetReviewWatch(ctx, rw.ID) |
There was a problem hiding this comment.
Suggestion: Error from GetReviewWatch is silently discarded — if the call itself fails for an unexpected reason (e.g., corrupted DB state), the nil check on line 72 still passes and the test reports a misleading failure message rather than the actual DB error.
Suggested change
| got2, _ := store.GetReviewWatch(ctx, rw.ID) | |
| got2, err2 := store.GetReviewWatch(ctx, rw.ID) | |
| if err2 != nil { | |
| t.Fatalf("GetReviewWatch after update: %v", err2) | |
| } |
| if err := store.DeleteReviewWatch(ctx, rw.ID); err != nil { | ||
| t.Fatalf("DeleteReviewWatch: %v", err) | ||
| } | ||
| got3, _ := store.GetReviewWatch(ctx, rw.ID) |
There was a problem hiding this comment.
Suggestion: Same pattern — GetReviewWatch error silently dropped. After a delete the contract is (nil, ErrWatchNotFound), not (nil, nil). Dropping the error accepts both outcomes as equivalent.
Suggested change
| got3, _ := store.GetReviewWatch(ctx, rw.ID) | |
| got3, err3 := store.GetReviewWatch(ctx, rw.ID) | |
| if err3 != nil && !errors.Is(err3, ErrWatchNotFound) { | |
| t.Fatalf("GetReviewWatch after delete: %v", err3) | |
| } |
| if err != nil || !ok { | ||
| t.Fatalf("first reserve: ok=%v err=%v", ok, err) | ||
| } | ||
| ok2, _ := store.ReserveReviewMRTask(ctx, "watch-1", "group/project", 42, "https://x") |
There was a problem hiding this comment.
Suggestion: The second-reserve error is silently discarded. The idempotent-dedup path is expected to return an error here (or ok=false with nil error, depending on the implementation). Dropping it means a non-nil error and ok=false both satisfy the ok2 check — it's worth asserting the exact semantic the store is meant to provide.
Suggested change
| ok2, _ := store.ReserveReviewMRTask(ctx, "watch-1", "group/project", 42, "https://x") | |
| ok2, err2 := store.ReserveReviewMRTask(ctx, "watch-1", "group/project", 42, "https://x") | |
| if err2 != nil { | |
| t.Fatalf("second reserve returned unexpected error: %v", err2) | |
| } |
| if err := store.CreateReviewWatch(ctx, rw); err != nil { | ||
| return nil, fmt.Errorf("create review watch: %w", err) | ||
| } | ||
| go s.initialReviewCheck(context.Background(), rw) |
There was a problem hiding this comment.
Suggestion: context.Background() makes this goroutine uncancellable — if the service shuts down mid-initial-check (e.g., during a fast restart), the goroutine continues running until it completes naturally. AGENTS.md requires goroutines to select on a cancellable context or shutdown signal. Consider threading the context from CreateReviewWatch's caller, or holding a service-level context (s.ctx) that is cancelled on shutdown.
Suggested change
| go s.initialReviewCheck(context.Background(), rw) | |
| go s.initialReviewCheck(ctx, rw) |
| if err := store.CreateIssueWatch(ctx, iw); err != nil { | ||
| return nil, fmt.Errorf("create issue watch: %w", err) | ||
| } | ||
| go s.initialIssueCheck(context.Background(), iw) |
There was a problem hiding this comment.
Suggestion: Same context.Background() pattern as service_watches.go:199 — the initial-check goroutine is detached from the caller's context and will outlive a shutdown in-flight. Consider passing ctx from CreateIssueWatch's parameter instead.
Suggested change
| go s.initialIssueCheck(context.Background(), iw) | |
| go s.initialIssueCheck(ctx, iw) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bring
internal/gitlabto full parity withinternal/github: PR/MR + issue watchers with background polling, write actions (merge, approve, labels/assignees, discussion notes), CI/pipeline status, action presets, and full HTTP + WS surfaces. Frontend gets matching types, API client, store slice, and core domain hooks.Backend (
apps/backend)gitlab_mr_watches,gitlab_review_watches,gitlab_review_mr_tasks,gitlab_issue_watches,gitlab_issue_watch_tasks,gitlab_action_presetsMergeMR,GetProjectMergeMethods,GetProtectedBranch,ListUserProjects,SearchProjects,SetMRLabels,SetMRAssigneesGetStats,CleanupAllReview/IssueTasks(cleanup-policy aware),TriggerMRSync, action presets CRUD with built-in defaultsmain.goGitLabMRFeedback,GitLabNewReviewMR,GitLabNewIssue,GitLabTaskMRUpdatedFrontend (
apps/web)ReviewWatch,IssueWatch,MRWatch,ActionPresets,Stats,ProjectMergeMethods,MRApproval,Pipeline,MRFeedback, etc.gitlab-api.ts): full HTTP wrappers for every new backend routegitlabReviewWatches,gitlabIssueWatches,gitlabMRWatches,gitlabActionPresets,gitlabStats,gitlabStatus(prefixed to avoid GitHub slice collision)useGitLabStatus,useGitLabStats,useGitLabReviewWatches,useGitLabIssueWatches,useGitLabActionPresets— mirror the GitHub hook shapesDecisions
workspace.repositoryfield for provider detection by hostVerification
go build ./...,go vet ./...,go test ./...,make lint— all greenpnpm typecheck,pnpm lint,pnpm test(2400 passing) — all greenTest plan
assigned_to_meMRs → trigger → confirm dedup row appears + event firesOut of scope / follow-ups
/gitlabpage shell andmr-list/issue-listcomponents exist as scaffolding — the new hooks are ready to plug into them in a follow-up)GITLAB_TOKENinjection into the agent runtime (/prskill detection)🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Preview Environment
36c9bd6